fix(route): add --desc flag + harden e2e gateway-group resolver#39
Merged
Conversation
The README documented `a7 route update <id> --desc "..."` but neither `route create` nor `route update` exposed a --desc flag; the field was only settable via -f. This made the documented invocation fail with 'unknown flag: --desc'. - Add --desc to flag-based create; wire to api.Route.Desc. - Add --desc to flag-based update with DescSet tracking so `--desc ""` explicitly clears the description (rather than being treated as 'unchanged'). - Unit tests cover the request-body wiring for both paths and the clear-via-empty-string case. - E2E TestRoute_DescFlagCRUD round-trips create / update / clear against a real instance. Closes #35.
📝 WalkthroughWalkthroughAdds ChangesRoute description flag support
E2E gateway-group resolution
🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR aligns the a7 route CLI with the README by adding a --desc flag to both route create and route update, including support for explicitly clearing the description on update, and adds unit/E2E coverage to prevent regressions.
Changes:
- Add
--desctoroute create(wired toapi.Route.Desc). - Add
--desctoroute updatewithDescSettracking so--desc ""is treated as an explicit update (clear) rather than “unchanged”. - Add unit tests for create/update request payloads plus an E2E CRUD round-trip test for set/update/clear.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/e2e/route_test.go | Adds an E2E regression test covering create/update/clear of desc via flags. |
| pkg/cmd/route/update/update.go | Adds --desc flag handling and DescSet tracking to support clearing. |
| pkg/cmd/route/update/update_test.go | Adds unit tests asserting desc is included/cleared in the PUT payload. |
| pkg/cmd/route/create/create.go | Adds --desc flag and includes Desc in the create request body. |
| pkg/cmd/route/create/create_test.go | Adds a unit test asserting --desc reaches the create request body. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/e2e/route_test.go`:
- Around line 425-427: The current check only asserts that cleared["desc"] is an
empty string which can falsely pass if "desc" is simply missing; after
performing the clear update, call the test helper that fetches the persisted
route (e.g., invoke the existing route-get helper or runCommand used elsewhere
in the test) and assert the fetched route's "desc" field is either absent or an
empty string; update the block around the cleared variable (the place
referencing cleared["desc"] and assert.Equal) to, if desc is present assert it's
empty, otherwise perform a follow-up route get and assert the returned object's
desc is empty or not present to ensure the clear persisted.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5141d829-883f-4f32-84fc-26e9e9eb2624
📒 Files selected for processing (5)
pkg/cmd/route/create/create.gopkg/cmd/route/create/create_test.gopkg/cmd/route/update/update.gopkg/cmd/route/update/update_test.gotest/e2e/route_test.go
- Drop the pre-registered name-based cleanup that referenced `routeID` (which is the route's name, not its id). The id-only cleanup registered after parsing `created["id"]` is the only correct one; the pre-reg was a no-op that masked leaks on early failures. - Treat absent, nil, and "" as a cleared desc via a small helper. API7 EE serializes a cleared field as `null`; the previous check produced a present-but-nil key and would have failed on a working CI environment. - Add a follow-up `route get` after the clear-update and assert the same on the persisted server state, not just the update response. Guards against the API echoing an empty desc without persisting it.
The previous resolver picked GET /api/gateway_groups list[0] and ignored both the env var and the group type. That made CI silently dependent on remote-server ordering: when the dev environment's first-returned group became an api7_ingress_controller-typed one, every mutating test failed 13 minutes in with a cryptic 'permission denied' from the CP middleware (`gateway_group.go:24`: ingress groups reject writes for any auth mode other than admin_key). - Treat A7_GATEWAY_GROUP=default as a literal name lookup, not a sentinel; honor any explicit name and fail fast at startup with the list of available names if no match. - Fall back path (empty name) now decodes Type and skips api7_ingress_controller groups instead of taking list[0]. - Startup log now prints the resolved name -> id mapping so the next environment drift is diagnosable from the first log line.
shreemaan-abhishek
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/e2e/setup_test.go`:
- Around line 140-145: The current normalization coerces both "" and "default"
into "default", preventing resolveGatewayGroupID from exercising its wanted ==
"" fallback; change the logic around the gatewayGroup/wanted variable so an
empty gatewayGroup remains an empty string (do not set wanted = "default" when
gatewayGroup == ""), i.e. remove or adjust the branch that normalizes "" into
"default" and pass the original gatewayGroup value into resolveGatewayGroupID
(refer to the local variable wanted and the call to resolveGatewayGroupID).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 09ae45b4-f8a9-440c-a5db-4dfce4a67214
📒 Files selected for processing (1)
test/e2e/setup_test.go
The previous version coerced both unset and "default" to "default", so resolveGatewayGroupID's non-ingress fallback (wanted == "") was unreachable. The docstring claimed unset → fallback but the code never delivered it. - Drop the package-level gatewayGroup = "default" default and the early env-override block; read A7_GATEWAY_GROUP once via os.Getenv and pass the literal value (incl. "") into the resolver. - Update the startup-log line to distinguish explicit-name resolution from the fallback path. - Update the package docstring to match the new behavior. CI workflow's A7_GATEWAY_GROUP=default is unchanged (still exact-name lookup); unset env now correctly hits the non-ingress fallback.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/e2e/setup_test.go`:
- Line 49: Remove the package-level mutable variable gatewayGroup and instead
create a test runtime/config struct (e.g., TestRuntime or TestConfig) that holds
gatewayGroup and any other shared test state; initialize this runtime in your
test setup (TestMain or a setup helper) and pass it explicitly into tests and
helper functions that previously read gatewayGroup (update signatures for any
functions that reference gatewayGroup to accept the runtime/config instance or
add methods on TestRuntime). Ensure all references to gatewayGroup in test/e2e
or helpers are replaced with runtime.gatewayGroup (or runtime.GetGatewayGroup())
so state is injected rather than global.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2d9837fe-b098-433f-8dd3-7e58fbccbd9b
📒 Files selected for processing (1)
test/e2e/setup_test.go
3 tasks
shreemaan-abhishek
added a commit
that referenced
this pull request
May 26, 2026
Reflects state as of 2026-05-26: - R2-1 (route --desc) fixed in PR #39 (closes #35) - R2-2 (route list UX) tracked as #42 (post-GA candidate) - R2-3 (credential positional id) tracked as #36 - R2-4 (global-rule --id) tracked as #37 - R2-5 (cosmetic) untracked, note only - Exit criteria last row flips to ✅ now that all bugs are tracked.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two changes:
1.
route--descflag (closes #35)The README documented
a7 route update <id> --desc "..."but neitherroute createnorroute updateexposed the flag; description was only settable via-f file.pkg/cmd/route/create: add--descflag wired toapi.Route.Desc.pkg/cmd/route/update: add--descwithDescSettracking so--desc ""explicitly clears (rather than being treated as unchanged).TestRoute_DescFlagCRUDround-trips create / update / clear with a follow-uproute getto verify the clear persisted server-side.2. Harden the e2e gateway-group resolver
The previous resolver picked
GET /api/gateway_groupslist[0]and ignored both theA7_GATEWAY_GROUPenv var and the group'stype. CI was silently dependent on remote-server ordering. When the dev environment's first-returned group became anapi7_ingress_controllerone, every mutating test failed 13 minutes in with the CP middleware'spermission denied: Unable to change resources in Ingress gateway group When auth type is not admin_key(CP:internal/pkg/middlewares/gateway_group.go:24, blocks non-admin_key writes to ingress-typed groups).A7_GATEWAY_GROUP=defaultis now a literal name lookup, not a sentinel.A7_GATEWAY_GROUP→ resolver still picks first, but skips ingress-controller groups in the fallback.Verified
go build/go vet(both with and without-tags e2e) clean.go test ./...green locally.TestRoute_DescFlagCRUDagainst API7 EE 3.9.12:--- PASS (0.93s).failed to resolve gateway group: gateway group "does-not-exist" not found; available: [default].CI
Today's CI failure on this PR is unrelated to either change — it's the same dev-env permission issue affecting every PR (PR #38 docs-only sees the same failure). Once the dev-env token / gateway-group is fixed, this PR's CI will go green; meanwhile the resolver hardening is precisely the thing that prevents this class of failure from happening again.
Closes #35. Part of #22.
Summary by CodeRabbit
New Features
Tests